Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Not supported property values #763

Merged
merged 3 commits into from
Oct 1, 2022
Merged

Not supported property values #763

merged 3 commits into from
Oct 1, 2022

Conversation

NiedziolkaMichal
Copy link
Member

@NiedziolkaMichal NiedziolkaMichal commented Apr 23, 2022 •
edited
Loading

This is fix for issues #441, #455 and #354.

Currently when property name or its value is not supported, they are simply not being applied without any warning. Only when user changes code content, warning might be shown, but this also doesn't work when just part of declaration block is not supported.
This is quite significant, because plenty of CSS property values are currently used in interactive examples, even though they are not supported at all. This is list of values not supported by Chrome, but still displayed to those users:

background-position-x: right 32px;
background-position-y: bottom 32px;
text-overflow: "-"
text-overflow: ""
color: hwb(1.5708rad 20% 10% / 0.7);
image-rendering: crisp-edges;
column-fill: balance-all;
text-transform: full-width;
text-transform: full-size-kana;
word-spacing: 120%;

I have made not supported values, but immediately shown to the users like this:
image
image

When interactive example is loaded, when reset button is used or when user types in something, it is checked whether whole declaration is property supported by the browser. This warning doesn't shown up, if unsupported property has vendor prefix or if ordinary property is not supported but its vendor prefix equivalent is supported correctly.

I have checked how my code works with every single declaration, currently being used in interactive-examples repository. You can check it by yourself by running this page: https://pastebin.com/73zGPWGG
There are 3 issues which require changing something in interactive-examples:

  1. "list-style-type: "�F44D"; // thumbs up sign." uses comment which is not valid in CSS.
  2. "transform: rotate3d(0)" is ignored by the browser and not applied at all, so this would need to be changed to something like 360 I think.
  3. "transform: translate3d(0);" same thing

Except for that, everything seems to be ok.

@wbamberg @schalkneethling

@wbamberg
Copy link
Collaborator

Again, I won't review the code here but the idea is pretty great.

The question really is whether we want to try to distinguish between:

  • the property not being applied because the syntax is invalid
  • the property not being applied because the browser doesn't support it

There's a bunch of discussion in mdn/interactive-examples#441 about this. The yellow cross mark is really intended for when people try typing a value and make a mistake, so they can see they made a mistake. Using the same mark for "not supported" might confuse that a little.

However:

  • it's hard for us to distinguish these two cases (not impossible, given the existence of the CSS-Tree validation (CSS Examples: Handle scenario where one property value is not supported interactive-examples#441 (comment)), but hard)
  • even if we could distinguish, it's not that obvious how we would communicate this to casual users in a way they would understand (it's fairly easy to have some generic "this doesn't work" indicator but harder to tell them why)
  • it's all a bit of an edge case really - tbh I doubt many people even try editing the CSS examples

So I think this is definitely better than what we have now!

@wbamberg
Copy link
Collaborator

I wonder if it would be better to show the "x" as well as the border all the time, and not only when an item is selected. I think if you opened https://developer.mozilla.org/en-US/docs/Web/CSS/text-transform in Chrome and saw only that the last two had the border, as opposed to the border and the "x", then it might not be very obvious what that means.

@schalkneethling
Copy link

Thank you so much for your contribution @NiedziolkaMichal. @mdn/core-dev when you have a moment, a review of the code here would be great. Thank you.

@schalkneethling schalkneethling added enhancement Improves an existing feature. community Contributions by the community labels Apr 26, 2022
@NiedziolkaMichal
Copy link
Member Author

I have made "X" be shown all the time, as you have suggested:
image

I think this feature could be further improved by displaying wavy red underline, under parts of the declaration that are invalid. So when a single keyword in shorthand property is not supported, user could quickly identify the problem. However such solution would require very complex implementation and might be an overkill for css interactive examples. I think something like that would be far more benefitial for JS/WAS examples, where simple syntax error might be hard to catch.

@NiedziolkaMichal NiedziolkaMichal mentioned this pull request Apr 27, 2022
Merged
@NiedziolkaMichal NiedziolkaMichal mentioned this pull request May 5, 2022
Merged
@wbamberg wbamberg mentioned this pull request May 9, 2022
Closed
@wbamberg
Copy link
Collaborator

@NiedziolkaMichal , sorry if this is an obvious question, but what will happen if the property as a whole isn't supported, rather than some particular values? Will it be the same as it is now, or will all the options get the cross? And what would we like to happen here?

I'm asking because of mdn/content#15824 - I really don't like the current behavior, but not sure I like the "all options get the cross" option much better...

@NiedziolkaMichal
Copy link
Member Author

In case one of used properties is not supported, example would have an orange outline and "X" on the right. However if base property is not supported, examples wouldn't be shown at all and warning introduced in PR #759 would be presented instead.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

queengooborg
queengooborg approved these changes Oct 1, 2022
View reviewed changes
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is LGTM, thank you!

@queengooborg queengooborg merged commit 85c0529 into mdn:main Oct 1, 2022
@NiedziolkaMichal
Copy link
Member Author

Thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@queengooborg queengooborg queengooborg approved these changes

Assignees
No one assigned
Labels
community Contributions by the community enhancement Improves an existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NiedziolkaMichal @wbamberg @schalkneethling @queengooborg